Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add standard name metadata to most EAMxx output variables #3105

Merged
merged 2 commits into from
Nov 20, 2024
Merged

Add standard name metadata to most EAMxx output variables #3105

merged 2 commits into from
Nov 20, 2024

Conversation

AaronDonahue
Copy link
Contributor

@AaronDonahue AaronDonahue commented Nov 8, 2024 •
edited
Loading

This commit brings EAMxx into CF-Compliance by adding standard_name metadata for a subset of the EAMxx output variables that have one.

Fixes #700

@mergify Mergify
Copy link
Contributor

mergify bot commented Nov 8, 2024 •
edited
Loading

Merge Protections

Your pull request matches the following merge protections and will not be merged until they are valid.

🟢 Enforce checks passing

Wonderful, this rule succeeded.

Make sure that checks are not failing on the PR, and reviewers approved

  • #approved-reviews-by >= 1
  • #changes-requested-reviews-by == 0
  • any of:
    • all of:
      • check-success="gcc-openmp / dbg"
      • check-success="gcc-openmp / fpe"
      • check-success="gcc-openmp / opt"
      • check-success="gcc-openmp / sp"
    • check-skipped={% raw %}gcc-openmp / ${{ matrix.build_type }}{% endraw %}
  • any of:
    • all of:
      • check-success="gcc-cuda / dbg"
      • check-success="gcc-cuda / opt"
      • check-success="gcc-cuda / sp"
    • check-skipped={% raw %}gcc-cuda / ${{ matrix.build_type }}{% endraw %}
  • any of:
    • all of:
      • check-success="cpu-gcc / ERS_Ln22.ne4pg2_ne4pg2.F2010-SCREAMv1.scream-small_kernels--scream-output-preset-5"
      • check-success="cpu-gcc / ERS_Ln9.ne4_ne4.F2000-SCREAMv1-AQP1.scream-output-preset-2"
      • check-success="cpu-gcc / ERS_P16_Ln22.ne30pg2_ne30pg2.FIOP-SCREAMv1-DP.scream-dpxx-arm97"
      • check-success="cpu-gcc / SMS_D_Ln5.ne4pg2_oQU480.F2010-SCREAMv1-MPASSI.scream-mam4xx-all_mam4xx_procs"
    • check-skipped={% raw %}cpu-gcc / ${{ matrix.test.short_name }}{% endraw %}
  • any of:
    • check-skipped=cpu-gcc
    • check-success=cpu-gcc

bartgol
bartgol reviewed Nov 8, 2024
View reviewed changes
components/eamxx/src/share/io/scream_io_utils.hpp Outdated
{"aero_g_sw" , "asymmetry_factor_of_ambient_aerosol_particles"},
{"pbl_height" , "atmosphere_boundary_layer_thickness"},
{"precip_liq_surf_mass" , "atmosphere_mass_content_of_liquid_precipitation"},
{"cldlow" , "cloud_area_fraction"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these really all the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not sure. I'm pretty sure I only grabbed standard_names I was able to confirm either on the CF website or from EAM. Let me check again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I found better standard names with a little more digging.

bartgol
bartgol previously approved these changes Nov 8, 2024
View reviewed changes
@AaronDonahue AaronDonahue requested a review from bartgol November 12, 2024 00:58
mahf708
mahf708 reviewed Nov 12, 2024
View reviewed changes
Copy link
Contributor

@mahf708 mahf708 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsolicited review: wouldn't we prefer to have this dict-type info in a yaml-like file somewhere in this folder (or elsewhere) instead of having it be part of the cpp code? I believe we could then just read the yaml file (or similar) to construct the map.

Of course, we can do that later, not necessarily in this PR ... I think it is tidier in the long run to have info like this stored in standalone files ...

@AaronDonahue
Copy link
Contributor Author

Unsolicited review: wouldn't we prefer to have this dict-type info in a yaml-like file somewhere in this folder (or elsewhere) instead of having it be part of the cpp code? I believe we could then just read the yaml file (or similar) to construct the map.

Of course, we can do that later, not necessarily in this PR ... I think it is tidier in the long run to have info like this stored in standalone files ...

I like this idea. I could probably make that a part of this PR. @bartgol , do you have any thoughts on this approach?

bartgol
bartgol previously approved these changes Nov 12, 2024
View reviewed changes
Copy link
Contributor

@bartgol bartgol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Maybe we could change "long_name" to "description", since having a short, a long, and a standard name seem too much...

@bartgol
Copy link
Contributor

bartgol commented Nov 12, 2024

Unsolicited review: wouldn't we prefer to have this dict-type info in a yaml-like file somewhere in this folder (or elsewhere) instead of having it be part of the cpp code? I believe we could then just read the yaml file (or similar) to construct the map.

Of course, we can do that later, not necessarily in this PR ... I think it is tidier in the long run to have info like this stored in standalone files ...

@jeff-cohere and I were working a while ago on a tool to enable this. You can see we have two databases in eamxx/data, containing yaml files for field names. We also developed string-matching tools in ekat, so that users can query the database to find what the real name is. We also thought about using it at runtime, but never had time to implement a robust approach. We can try in a following PR. Are you volunteering? ;-)

@bartgol bartgol added the CI: automerge WARNING: Still in an experimental phase label Nov 12, 2024
@mahf708
Copy link
Contributor

mahf708 commented Nov 12, 2024

Are you volunteering? ;-)

Yes! But only if I get to work with you and @jeff-cohere once we get to it (in a few weeks/months) ;)

@AaronDonahue AaronDonahue force-pushed the aarondonahue/io/add_standard_name_metadata branch from ccdcfb4 to 42e1119 Compare November 12, 2024 21:22
@AaronDonahue AaronDonahue requested a review from bartgol November 12, 2024 21:22
@bartgol bartgol dismissed their stale review November 12, 2024 21:52

The merge-base changed after approval.

@bartgol bartgol force-pushed the aarondonahue/io/add_standard_name_metadata branch from 42e1119 to 68c46f4 Compare November 12, 2024 21:52
@bartgol bartgol force-pushed the aarondonahue/io/add_standard_name_metadata branch from 68c46f4 to b81521f Compare November 12, 2024 21:54
@bartgol bartgol force-pushed the master branch 2 times, most recently from 6318f41 to db5b35d Compare November 13, 2024 21:48
AaronDonahue and others added 2 commits November 19, 2024 10:12
@AaronDonahue
Add standard name metadata to most EAMxx output variables
2b50dbb 
This commit brings EAMxx into CF-Compliance by adding `standard_name`
metadata for a subset of the EAMxx output variables that have one.
@AaronDonahue
fix standard names for low/med/hgh cloud area fraction types
d44a616
@AaronDonahue AaronDonahue force-pushed the aarondonahue/io/add_standard_name_metadata branch from b81521f to d44a616 Compare November 19, 2024 18:14
@AaronDonahue AaronDonahue requested a review from bartgol November 19, 2024 18:14
@mergify mergify bot merged commit c98f074 into master Nov 20, 2024
20 checks passed
@mergify mergify bot deleted the aarondonahue/io/add_standard_name_metadata branch November 20, 2024 20:25
@mahf708
Copy link
Contributor

mahf708 commented Nov 20, 2024 •
edited
Loading

@bartgol uh oh... mergify merges without up-to-date approval, nice?!

@bartgol
Copy link
Contributor

bartgol commented Nov 20, 2024

@bartgol uh oh... mergify merges without up-to-date approval, nice?!

I'm guessing that pushing new commits is not invalidating the check "#approved-reviews-by >= 1"? Seems odd that mergify looks for "any approval given during the lifetime of the PR", rather than checking that the last commit was approved...

@bartgol
Copy link
Contributor

bartgol commented Nov 20, 2024

It appears that mergify does indeed that: so long as the PR was approved "at some point", that check passed. There's a mergify action that allows to dismiss reviews though. I want to see if we can use it to dismiss stale reviews. If not, then byebye mergify

@rljacob
Copy link
Member

rljacob commented Nov 20, 2024

I think there's a github branch protection setting that might help: "Dismiss stale pull request approvals when new commits are pushed. New reviewable commits pushed to a matching branch will dismiss pull request review approvals."

@mahf708
Copy link
Contributor

mahf708 commented Nov 20, 2024

I think there's a github branch protection setting that might help: "Dismiss stale pull request approvals when new commits are pushed. New reviewable commits pushed to a matching branch will dismiss pull request review approvals."

I believe we have this already in this repo...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mahf708 mahf708 mahf708 left review comments

@rljacob rljacob Awaiting requested review from rljacob

@bartgol bartgol Awaiting requested review from bartgol

Assignees
No one assigned
Labels
CI: automerge WARNING: Still in an experimental phase I/O
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[AD] Implement CF-Compliance Checking in Field Repo
4 participants
@AaronDonahue @bartgol @mahf708 @rljacob